Fix removeUnused mode not loading plugin checks and alt names by kelvinou01 · Pull Request #278 · palantir/suppressible-error-prone

kelvinou01 · 2025-12-12T18:14:05Z

Before this PR

A few bugs in suppressible-error-prone:

AllBugCheckers wasn't loading any plugin checks
RemoveUnused mode didn't understand suppressions using alt-names, e.g. SuppressWarnings("unused"), leading it to overzealously remove suppressions

After this PR

AllBugCheckers loads plugin checks
RemoveUnused mode understands alt-names

==COMMIT_MSG==
Fix removeUnused mode not loading plugin checks and alt names
==COMMIT_MSG==

Possible downsides?

Probably next PR: RemoveUnused should not remove human-made suppressions. For example, a human-made suppression on SafeLoggingPropagation for a method which is safe to log. RemoveUnused + Apply should not remove the suppression and add the @Unsafe annotation.

If needed, human-authored suppressions can be removed on an allow-list basis. But the correct and performant removal of robot-added suppressions should be the focus here.

changelog-app · 2025-12-12T18:14:09Z

Generate changelog in `changelog/@unreleased`

Type (Select exactly one)

Feature (Adding new functionality)
Improvement (Improving existing functionality)
Fix (Fixing an issue with existing functionality)
Break (Creating a new major version by breaking public APIs)
Deprecation (Removing functionality in a non-breaking way)
Migration (Automatically moving data/functionality to a new system)

Description

Fix removeUnused mode over- and under-removing suppressions

Check the box to generate changelog(s)

Generate changelog entry

…names

changelog-app · 2025-12-15T11:57:45Z

✅ Successfully generated changelog entry!

Need to regenerate?

Simply interact with the changelog bot comment again to regenerate these entries.

📋Changelog Preview

🐛 Fixes

Fix removeUnused mode over- and under-removing suppressions (#278)

kelvinou01 · 2025-12-15T18:32:22Z

        '''.stripIndent(true)

        when:
-        runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused=ArrayToString')


RemoveUnused doesn't take arguments — this argument was being ignored

aldexis

I tried to have a look, but I'm not familiar enough with the remove unused logic to confidently review the actual logic changes. Best to have @CRogers have an eye on this as well

aldexis · 2025-12-16T14:51:20Z

+@BugPattern(
+        severity = BugPattern.SeverityLevel.ERROR,
+        summary = "This check is meant only for testing suppressible-error-prone functionality")
+public final class ShouldNotReturn extends BugChecker implements MethodTreeMatcher {


Did you use this anywhere?

aldexis · 2025-12-16T15:08:52Z

-                ServiceLoader.load(BugChecker.class).stream().map(ServiceLoader.Provider::get);
-        Stream<String> pluginCheckNames =
-                StreamEx.of(pluginChecks).flatMap(bugchecker -> bugchecker.allNames().stream());
+    private static Supplier<Map<String, Set<String>>> canonicalToAllNames = VisitorState.memoize(state -> {


Suggested change

private static Supplier<Map<String, Set<String>>> canonicalToAllNames = VisitorState.memoize(state -> {

private static final Supplier<Map<String, Set<String>>> canonicalToAllNames = VisitorState.memoize(state -> {

aldexis · 2025-12-16T15:11:03Z


-    public static Set<String> allBugcheckerNames() {
-        return cachedAllBugcheckerNames.get();
+    private static Supplier<Set<String>> allNames =


Suggested change

private static Supplier<Set<String>> allNames =

private static final Supplier<Set<String>> allNames =

aldexis · 2025-12-16T15:11:11Z

+                    .flatMap(entry -> entry.getValue().stream())
+                    .collect(Collectors.toSet()));
+
+    private static Supplier<Map<String, Set<String>>> nameToPossibleCanonicalNames =


Suggested change

private static Supplier<Map<String, Set<String>>> nameToPossibleCanonicalNames =

private static final Supplier<Map<String, Set<String>>> nameToPossibleCanonicalNames =

aldexis · 2025-12-16T15:14:13Z

+                    .flatMap(entry -> entry.getValue().stream())
+                    .collect(Collectors.toSet()));
+
+    private static Supplier<Map<String, Set<String>>> nameToPossibleCanonicalNames =


Is it possible to have multiple checks share the same alt name? I imagine so, though it's also pretty surprising

aldexis · 2025-12-16T15:16:29Z

+                                declaration, state, suppression -> !AllErrorprones.allBugcheckerNames(state)
+                                        .contains(suppression));


You were previously removing the for-rollout prefix and aren't doing so anymore. Is that intentional?

aldexis · 2025-12-16T15:22:56Z

+            String existingSuppression = AnnotationUtils.annotationStringValues(
+                            SuppressWarningsUtils.getSuppressWarnings(declaration)
+                                    .get())
+                    .filter(suppression -> allNames.contains(SuppressWarningsUtils.stripForRollout(suppression)))
+                    .collect(MoreCollectors.first())
+                    .get();
+            FIXES.getExisting(declaration).addSuppression(existingSuppression);


Shouldn't you keep all the ones that might already exist, rather than just the first one? (especially the non-rollout ones)

aldexis · 2025-12-16T15:24:26Z

+            String existingSuppression = AnnotationUtils.annotationStringValues(
+                            SuppressWarningsUtils.getSuppressWarnings(declaration)
+                                    .get())
+                    .filter(suppression -> allNames.contains(SuppressWarningsUtils.stripForRollout(suppression)))
+                    .collect(MoreCollectors.first())
+                    .get();


Could this leverage suppressionsOn in some way?

CRogers · 2025-12-16T18:01:44Z

+                .map(Provider::get)
+                .collect(Collectors.toList());
+        EntryStream<String, Set<String>> pluginCheckNames =
+                StreamEx.of(pluginChecks).mapToEntry(BugChecker::canonicalName, BugChecker::allNames);


BugChecker#allNames will return for-rollout:Name if we've patched the errorprone classes in the artifact transform.

Which may be what we want...

CRogers · 2025-12-16T18:02:10Z

+        // Use the same classloader that Error Prone was loaded from to avoid classloader skew
+        // when using Error Prone plugins together with the Error Prone javac plugin.
+        JavacProcessingEnvironment processingEnvironment = JavacProcessingEnvironment.instance(state.context);
+        ClassLoader loader = processingEnvironment.getProcessorClassLoader();


I wonder if there's any real difference between this and state.getClass().getClassLoader()? The latter does not require interacting with so many compiler APIs or module imports.

removeUnused mode loads plugin checks correctly, and understands alt …

e507d60

…names

kelvinou01 force-pushed the okelvin/fix-remove-unused branch from 082b6c1 to e507d60 Compare December 15, 2025 11:32

kelvinou01 changed the title ~~Fix removeUnused mode over- and under-removing suppressions~~ Fix removeUnused mode not loading plugin checks, and not respecting alt names Dec 15, 2025

kelvinou01 changed the title ~~Fix removeUnused mode not loading plugin checks, and not respecting alt names~~ Fix removeUnused mode not loading plugin checks and alt names Dec 15, 2025

kelvinou01 added autorelease merge when ready labels Dec 15, 2025

kelvinou01 requested review from CRogers and aldexis and removed request for CRogers December 15, 2025 18:31

kelvinou01 mentioned this pull request Dec 15, 2025

Add the preferKeepingExistingSuppression extension property to shunt removeUnused mode for non-standard checks #279

Open

kelvinou01 commented Dec 15, 2025

View reviewed changes

kelvinou01 requested a review from CRogers December 15, 2025 18:44

aldexis reviewed Dec 16, 2025

View reviewed changes

CRogers reviewed Dec 16, 2025

View reviewed changes

	private static Supplier<Map<String, Set<String>>> canonicalToAllNames = VisitorState.memoize(state -> {
	private static final Supplier<Map<String, Set<String>>> canonicalToAllNames = VisitorState.memoize(state -> {

	private static Supplier<Set<String>> allNames =
	private static final Supplier<Set<String>> allNames =

		declaration, state, suppression -> !AllErrorprones.allBugcheckerNames(state)
		.contains(suppression));

Conversation

kelvinou01 commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before this PR

After this PR

Possible downsides?

Uh oh!

changelog-app Bot commented Dec 12, 2025 • edited by kelvinou01 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Generate changelog in changelog/@unreleased

Uh oh!

changelog-app Bot commented Dec 15, 2025

✅ Successfully generated changelog entry!

Need to regenerate?

📋Changelog Preview

🐛 Fixes

Uh oh!

kelvinou01 Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aldexis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kelvinou01 commented Dec 12, 2025 •

edited

Loading

changelog-app Bot commented Dec 12, 2025 •

edited by kelvinou01

Loading

Generate changelog in `changelog/@unreleased`

kelvinou01 Dec 15, 2025 •

edited

Loading